Skip to content

[Ledger] Add GIN index for ledger item memo search#14353

Open
garyhtou wants to merge 5 commits into
mainfrom
garyhtou/ledger-item-memo-search-index
Open

[Ledger] Add GIN index for ledger item memo search#14353
garyhtou wants to merge 5 commits into
mainfrom
garyhtou/ledger-item-memo-search-index

Conversation

@garyhtou

@garyhtou garyhtou commented Jul 18, 2026

Copy link
Copy Markdown
Member

Note

This PR was written by Claude Code. Gary directed the investigation and reviewed the changes, but the diagnosis, code, and benchmarks below were produced by Claude.

Summary of the problem

Searching a ledger (e.g. /:event_id/ledger?q=...) was slow regardless of how narrow the search was. Ledger::Item.search_memo (a pg_search_scope, dictionary "simple", no using: set) had no index behind it, so every search computed to_tsvector() on the fly via a sequential scan — and that scan wasn't even scoped to the ledger being viewed, since Ledger::Query's $search branch (app/models/ledger/query.rb) ran the search as its own subquery across every ledger_items row in the system, only intersecting with the current ledger's items afterward.

Describe your changes

Commit 1 — GIN index. Added a functional GIN index matching the exact expression pg_search generates:

add_index :ledger_items, "to_tsvector('simple', coalesce(memo, ''))", using: :gin

Confirmed via EXPLAIN this changes the plan from a Parallel Seq Scan (with Rows Removed by Filter in the hundreds of thousands) to a Bitmap Index Scan.

Commit 2 — scope the $search subquery to the queried ledgers. The search subquery itself had no knowledge of which ledger(s) it was being run for, so it always searched system-wide before the outer query intersected the results down to the right ledger. Stashed the ledger scope on the Ledger::Query instance (set in execute() before apply_query runs) so the $search branch can scope its own subquery the same way execute() scopes the final result set.

Worth being precise about what this is and isn't: this is a performance fix, not a security fix. The pre-existing outer scoping clamp in execute() already correctly restricted the final result set in the old code — I confirmed this empirically by reverting the change and running the new test suite against it (see commit 3): the "no data leaks across ledgers" test passes on both old and new code. The subquery being unscoped only meant Postgres did unnecessary system-wide work before the correct final intersection, not that wrong data was ever returned.

Commit 3 — test coverage. $search had zero existing test coverage. Added:

  • basic $search functional coverage
  • a ledger-scoping test for $search (passes on old code too, per above — kept as general regression coverage)
  • a SQL-shape test that actually discriminates old vs. new code (asserts the search subquery contains its own ledger_mappings scope, not just the outer result set) — verified this one fails against the pre-fix code and passes against the fix

Benchmarks

Searching "Gary Tou" (48 real matches — my own name, chosen so there's nothing to redact) and "Luke Oldenburg" (the term from the original slow-request report, 45 matches):

Before After (index only) After (index + scoped subquery)
search_memo("Gary Tou") 364ms 2.9ms
search_memo("Luke Oldenburg") 361ms 3.6ms
Full search + paginate, as the controller does it (Ledger::Query#execute(...).page(1).per(25)) 477.6ms ~16-30ms warm

~15-120x faster depending on what's measured. 63 examples in spec/models/ledger/query_spec.rb pass (58 pre-existing + 5 new).

Out of scope

Doesn't address fuzzy/typo-tolerant search — the current search is exact-token match only ("simple" dictionary, no stemming, no pg_trgm). That would be a separate, larger change (different extension, different index type, different match semantics) if ever wanted.

Gary separately flagged that Ledger::Query#execute could be restructured so a ledger-scoped base relation is established up front and all predicates build on top of it, rather than scoping being bolted on per-predicate as this PR does for $search. Agreed that's a good idea architecturally — it would make this whole class of "a new predicate type forgets to scope itself" bug impossible by construction rather than relying on each handler remembering to. Left for a separate PR; one thing to watch for there: $or/$nor branches (query.rb, lines building sub-relations from Ledger::Item.all directly) reset to a fresh base relation rather than building on the enclosing one, so that base needs to be the ledger-scoped Ledger::Item.all, not a literal unscoped one, or $or/$nor branches would lose the ledger scope.

garyhtou and others added 3 commits July 18, 2026 00:54
Ledger::Item.search_memo (pg_search_scope, dictionary "simple") had no
index behind it, so every memo search — e.g. a ledger page's `q=`
filter — computed to_tsvector() on the fly across every ledger_items
row in the system (not just the current ledger's), via a sequential
scan.

Added a functional GIN index matching the exact expression pg_search
generates. Benchmarked on real production data: searching "Gary Tou"
(48 real matches) went from 364ms median to 2.9ms; "Luke Oldenburg"
(45 matches) went from 361ms to 3.6ms. Confirmed via EXPLAIN that
Postgres now uses a Bitmap Index Scan instead of a Parallel Seq Scan.

spec/models/ledger/query_spec.rb and item_spec.rb pass unchanged (85
examples) — this only changes plan/speed, not matching behavior.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Ledger::Query's $search branch ran Ledger::Item.search_memo(operand)
unscoped — a full-text search across every ledger's items in the
system — then intersected the result with the ledger-scoped outer
relation. The GIN index added in the prior commit makes this fast
regardless, but the subquery itself had no knowledge of which
ledger(s) it was being run for.

Stashed the ledgers/all_ledgers scope on the instance in execute() (set
before apply_query runs) so the $search branch can apply the same
Ledger::Mapping scoping to its own subquery that execute() applies to
the final result set. Confirmed via .to_sql that the search subquery
now includes the ledger_mappings scope, and end-to-end (search +
paginate) on real production data dropped from 477ms to ~16-30ms warm.

spec/models/ledger/query_spec.rb, item_spec.rb, and
ledgers_controller_spec.rb pass unchanged (87 examples).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
All three independent reviewers of the previous commit flagged the
same gap: zero test coverage for $search in query_spec.rb.

Added basic $search functional coverage, plus ledger-scoping tests.
Verified each new "ledger scoping" test actually discriminates
old vs. new code by running it against the pre-fix version of
query.rb: the cross-ledger result-set test passes on both (there was
never an actual data leak — the outer scope already covered it, this
was a performance fix, not a security one), so the coverage that
actually pins the change down asserts on the generated SQL shape (two
separate ledger_mappings subqueries: one for the search, one for the
final result — vs. one on the old code).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@garyhtou
garyhtou marked this pull request as ready for review July 18, 2026 08:48
@garyhtou
garyhtou requested a review from a team July 18, 2026 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants